[Stepping] Correctness sweep across the optimization loop: measurement integrity, phase budgets, resume safety, and a large dead-code removal - #1188
Merged
Conversation
GridVariant.extra_envs was filtered for control-plane credentials only, so a proposed variant could carry LD_PRELOAD, PATH or PYTHONPATH into the benchmark config. That is inert while the bypass serving path exports nothing but the GPU pin, and becomes a live injection surface the moment it exports the mapping. An allowlist is not usable here: the sweep, conc-sweep and shape-capture grids build variants from code and legitimately set CONC/ISL/OSL/NUM_PROMPTS/RUN_EVAL/ PORT/TP/MAX_MODEL_LEN, then read them back. Block the shell/loader hijacks and the control-plane credentials instead, and leave workload pins alone. filter_benchmark_env_mapping loses both call sites and is removed; the YAML persistence site keeps its credential-only scope through the shared filter, which also gains it a key-syntax check. Co-authored-by: Cursor <cursoragent@cursor.com>
…server The bypass serving path hand-picked ROCR_VISIBLE_DEVICES out of benchmark.envs and dropped the rest, so a candidate whose only difference from the baseline was an env launched a byte-identical server: same argv, same environment. The measured delta was run-to-run noise, the default 1% keep threshold promoted it, and the env then landed in current_best and in the reference script handed to the user. Kernel bundles and framework levers are activated by exactly such switches, so their attribution was measuring nothing. The scriptable path already exported the whole mapping. Converge both onto one branchless helper that layers mappings over the parent env rather than adding a third hand-picked key. _run_subprocess and the GEAK sweep share the same shape and now go through it as well. Behaviour change: every shipped config carries a static PATH under benchmark.envs, which now reaches the server and wins over the caller's inherited-and-prepended value. That is what the Magpie backend has always done, and bypass documents itself as accepting the same environment contract. Co-authored-by: Cursor <cursoragent@cursor.com>
unset_envs popped any name it was given, so a variant could drop TP, CONC, ISL, RUN_EVAL or the GPU mask from the materialized config. That does not toggle a knob, it retargets the benchmark: dropping TP also shrinks the Ray lease to a single GPU, because the lease size is read back out of benchmark.envs. Reuse BLOCKED_EXTERNAL_ENV_NAMES, the list that already says which names an untrusted source may not set. Setting and unsetting are two directions of the same change, so they answer to one list. Ordinary tuning knobs stay removable. Co-authored-by: Cursor <cursoragent@cursor.com>
Both statements described the bypass backend as honouring a single hand-picked key while Magpie re-exported the whole benchmark.envs mapping. Bypass now exports the mapping too, so the only remaining difference is that it also uses the interpreter to launch the server. Co-authored-by: Cursor <cursoragent@cursor.com>
…itial toolkit Introduced in 9167263 (2026-04-30) as the self-validation driver for the then-new kernel-agent, when no KERNEL phase existed to prove the toolkit on a real model. Its purpose window closed in three steps: 068c667 dropped the run_baseline.sh step so it stopped being end-to-end and required a trace produced elsewhere, ca99ad7 removed the backend-drop logic and left backends_dropped永 empty behind a misleading error string, and #840 emptied the --backends default so a plain invocation only ever raised. What it was built to prove is now covered by the orchestrator's own KERNEL phase and ci-e2e.yml. No in-repo caller; operator confirmed no out-of-repo caller. Its artifacts parallel_e2e_summary.{json,md} had no reader either -- the breakdown collector reads reports/kernel_optimization_summary.json and reports/conc_sweep_summary.json by exact relative path and no glob reaches them. The deletion also closes an aggregation bug rather than fixing it: every attempt's exception was flattened into a row-level failed, but the aggregate printed "status": "succeeded" and returned 0 unconditionally, and never wrote a status key into the summary on the success path at all. Measured with all attempts failing: 2/2 failed, top-level succeeded, exit 0. load_env_file was a second implementation of the credential-alias derivation in tools/backends/ray_runtime.py:452-472 (same rule, verbatim the same comment). That one stays and is pinned by seven tests in test_ray_runtime_env.py, four of which cover the "GEAK aliases are never derived, forwarded verbatim" boundary the deleted test file covered with three -- so the guardrail is preserved. test_kernel_agent_live.py keeps its mirrored derivation and now points at ray_runtime; whether that dormant opt-in harness should live is a separate call. Coverage is unaffected: the module was already in [tool.coverage.run].omit, so its tests contributed nothing to the gate; the dangling omit entry goes too. _collective_names, _io_utils and _paths all keep other consumers. Co-authored-by: Cursor <cursoragent@cursor.com>
… warnings Aggregate the dropped-key warning into one line per variant rather than one per key, collapse a comment that was duplicated verbatim at both unset sites, and record the new filtering in the docstrings that still promised unconditional behaviour. Co-authored-by: Cursor <cursoragent@cursor.com>
ZhengGong-amd
marked this pull request as draft
August 14, 2026 03:12
CI E2E report — ✅ Succeeded
|
The relaunch passed both --resume and --resume-from. Drop the redundant flag and re-anchor the pgrep pattern that resolves the resumed pid, so the monitor keeps working once bare --resume stops being accepted. Co-authored-by: Cursor <cursoragent@cursor.com>
Bare --resume scanned every model subtree under $USER_DATA_PATH and attached to whichever session had the newest timestamp, then re-exported that session's model as MODEL_PATH without a word. On a shared workspace that silently continues someone else's run on a different model, which is what SKILL.md already forbids under "never guess by timestamp". --resume-from becomes the only way to resume, so the session is always named by the operator. The flag it used to imply is gone; a stale --resume now fails at argument parsing instead of picking a session. Also retires the workspace-root fallback (unreachable: the sole production make_session_dir call always passes a model, so no manifest ever lands there) and the quantization resume guard (unreachable: the prelude only runs on the fresh-launch path). Co-authored-by: Cursor <cursoragent@cursor.com>
SKILL.md forbade guessing a session dir from timestamps in three places while its own resume section documented exactly that, and described the scan as model-scoped when it never was. Drop the contradiction and name --resume-from as the only resume entry point across the skill files, references, README, and the how-to/reference docs. CHANGELOG and release notes keep their original wording; they record what shipped at the time. Co-authored-by: Cursor <cursoragent@cursor.com>
…d placeholder The blank-id fallback mapped every empty task/session/kernel id onto a shared "unknown" directory. Nothing could reach it: registry task ids are uuid4 hex, and every other call site either substitutes its own placeholder or returns early, so the bucket had no producer and no reader beyond three assertions. Keeping it also forced an early return that skipped the traversal check, so a containment helper carried a bypass branch. Blank is now refused alongside path-like values, which is the only correct terminal behaviour here: pathlib swallows an empty segment, so a blank id would resolve to the parent action directory and collide with every sibling task. Co-authored-by: Cursor <cursoragent@cursor.com>
…rate_patch Each of these three sites substituted its own name for a blank task id, but the id is a registry uuid4 hex that cannot be blank, so the substitutes were unreachable. Three different placeholders for the same non-existent case only obscured that the workspace is always keyed by the real task. The path helper now refuses a blank component outright, so no local fallback is needed to keep the workspace off the shared action directory. Co-authored-by: Cursor <cursoragent@cursor.com>
argparse resolves --resume as a prefix of --resume-from rather than rejecting it outright, so pin the guarantee that matters: every legacy invocation, including the one already-deployed monitor copies send, exits 2 instead of choosing a session. Co-authored-by: Cursor <cursoragent@cursor.com>
…used RunnerContext.task is typed Task and task_id is a required field, so the getattr default could never fire — it only implied the id might be absent. The path helper refuses a blank component outright now, so nothing downstream needs a guard of its own. Also trims the two comments that grew with the previous change back to the length they had before it. Co-authored-by: Cursor <cursoragent@cursor.com>
Trim the duplicated pin comment and the session-discovery chain that the skill docs already carry, and drop two tests that asserted a symbol was absent or that argparse round-trips a string rather than any behaviour of ours. Restores the blank line the quantization guard removal ate. Co-authored-by: Cursor <cursoragent@cursor.com>
The per-launch dir name was a second-granular UTC timestamp, so two launches of one model in the same second resolved to one session dir. That name is also the de-facto session id (session_dir.name feeds KB fact writes, critic requests and per-session sinks), so a collision merged two runs' identity as well as their artifacts. Uniqueness now comes from a random suffix; the fixed-width timestamp prefix keeps lexical order chronological. Co-authored-by: Cursor <cursoragent@cursor.com>
Subprocesses are spawned with explicit cwds of their own, so a relative $USER_DATA_PATH sent parent and child to different workspace roots while both believed they agreed. Absolutising on read cannot fix that — each process re-expands the relative value against its own cwd — so the launcher now rewrites the env var once, before the parser defaults or any session path derive from it. The manifest snapshot drops its own duplicate env read and goes through workspace_root(), which is the same value with one less way to disagree. Co-authored-by: Cursor <cursoragent@cursor.com>
…omments The dir name gained a random suffix but four places still spelled the old shape: two skeleton/layout comments and the layout tree plus literal session path example in SKILL.md and references/paths.md, both of which ship as agent instructions. Also drops what the code already says: the whitespace-strip the authority itself does not do, two assertions implied by their neighbours, and the rationale paragraphs. Co-authored-by: Cursor <cursoragent@cursor.com>
…ver source
The session-scoped endpoints were keyed off SharedState.session_id, which
build_session_id() generates locally as "{model}_{ts}_{uuid8}". Nothing ever
registers that id with the robustness server -- the client is GET-only and the
platform's claw_session_id is never forwarded to the agent -- so
/api/v1/sessions/{id}/{pods,events,summary} could only ever 404. Those 404s were
folded into empty results, which the source then returned as a successful tick.
Pod discovery already had a working route: the workload_uid hierarchy lookup,
whose rows were merged into session_pods anyway. That is now the only route.
Drops list_session_pods, list_session_events, get_session_summary,
_extract_session_id, the now-unused _to_iso helper and events_limit knob, and
the SourceData.session_events / session_metrics fields, which had no readers.
_merge_pods loses its session-side half and becomes _wrap_hierarchy_pods.
Co-authored-by: Cursor <cursoragent@cursor.com>
The snapshot carried a second, weaker copy of the session id: parsed out of the prompt text while the authoritative one already arrives on the request and is handed to build_reactor_components and the emit payload. Its only reader was the server source's session lookup, removed in the previous commit. The runtime CLI's fallback existed solely to backfill that field, and rebuilt the context by hand-listing members. That list was complete when written (the snapshot had exactly 7 fields) but the snapshot has since grown to 19, so a tick taking the fallback silently lost stop_reason, the wall-clock budget, validated gain, tick, macro_cycle and the rest -- muting postmortem finalization and the deadline wind-down. Deleting the branch outright removes both the shadow field and the field-loss bug. Replaces the fallback's vacuous test (it asserted an emit key sourced from the request, so it passed with the branch deleted) with one that pins the real contract: every parsed shared-state field reaches the reactor. Co-authored-by: Cursor <cursoragent@cursor.com>
The symptom read SourceData.session_summary, whose only producer was the session summary endpoint removed earlier. With no producer left it could never fire, so the rule, the field, and the HealthConfig slot that existed solely to tune it all go. SignalSpec already allows a config-less row, so the classifier keeps calling evaluate_health_signals for pod_not_running with no other change. Co-authored-by: Cursor <cursoragent@cursor.com>
…othing A tick that fetched no pods, no faults and no GPU metrics still returned a SourceData, which the DegradeRouter counted as a healthy primary. The local probe was then never consulted, so GPU leak, log-error, disk, ray and state-integrity signals went silent alongside the cluster ones -- and silently, since sources_used still named the server. Failing the tick instead lets the router degrade after its usual threshold and log the transition. Deployments with no workload uid configured will now fall back to the local probe rather than reporting a clean bill of health. Co-authored-by: Cursor <cursoragent@cursor.com>
The `failed -> running` retry edge was removed when retries moved to a fresh idempotency key, but the hand-listed TERMINAL_STATES kept its old membership. A failed row therefore has no outgoing transition at all yet reports as still live, and every "is this row finished?" guard reads it wrong. Three of them wedge. The watermark roofline gate never releases, so a roofline reclaimed by the TTL watchdog -- which reports no result, leaving the release as the only thing that could clear the marker -- gates every later refresh; the marker is persisted state, so the next resume inherits the wedge. Resume recovery never clears enablement_validation_pending for a failed revalidation. And the FRAMEWORK authoring pump keeps re-selecting a candidate whose specialist row is dead, logging a dispatch every tick while nothing runs. Deriving the set from _TRANSITIONS instead of restating it means the two cannot drift apart again. proposals.py had already worked around the stale constant with a private copy that did include failed, so it now imports the shared one; the import left unused in framework.py goes with it. The gate's failed branch was never exercised -- the existing test only set up a succeeded row -- so the two near-identical gate tests collapse into one parametrized case that pins all five states. Co-authored-by: Cursor <cursoragent@cursor.com>
…nt log The pending-integrate recovery treated "no KEEP found" and "could not read the event log" as the same outcome: a swallowed scan error left kept_res as None, which fell through to reverse-applying every recorded patch and then cleared the sentinel unconditionally, so a transient DB error during the first resume after a hard crash would destroy a validated patch, discard the attempt venv, report the loss as a fix, and leave nothing for a later resume to retry. Track whether the scan completed and bail out with a warning that retains the sentinel when it did not, so the destructive branch runs only on evidence. Co-authored-by: Cursor <cursoragent@cursor.com>
…ng it The GEAK branch never measured anything: it derived a throughput as baseline_tput * best_speedup and wrote that straight onto current_best, so a GEMM speedup of 1.1 would overwrite a current_best already carrying an explore win of 1.5x and rewrite the session's validated cumulative gain as the single GEMM's speedup. Only forge results reached the per-tuner validator, which already measures each candidate against current_best via integrate_handler. Reduce both result shapes to a common candidate list, route every backend through that validator, and delete the synthetic-throughput promoter. The validator is no longer forge-specific, so it and its log lines lose the forge name; stack entries and current_best now carry the backend that produced them. Co-authored-by: Cursor <cursoragent@cursor.com>
integrate, geak_e2e, forge GEMM, fusion and warm_replay each hand-built their own current_best and stack entry. Four of them replaced current_best outright, so a KEEP could drop the cumulative server args or resurrect an env an earlier ablation had removed, and only _lift_to_current_best merged onto the previous config or applied unset_envs at all. All five now build a winner dict and hand it to the lift, which gains an entry_extra parameter for the artifact and provenance handles the breakdown collectors join on, plus final_overlay so the active overlay survives a later winner that carries none. current_best keeps only config; tuned_file, engine, patch_path, geak_alignment and the rest move to the stack entry or go away where nothing read them. Two behaviour changes fall out. integrate and fusion had no anchor guard, so a KEEP measured below the established recipe used to lower current_best; the lift now refuses it. warm_replay hand-appended a rounded percentage to gain_per_stack_entry instead of calling append_stack_gain_entry, so that list was not computed the same way as every other entry's. Baseline and profile still assign current_best directly, which stays correct: baseline only writes it while the stack is empty, and profile copies the existing config and re-stamps the measured tput. Co-authored-by: Cursor <cursoragent@cursor.com>
Resume compared current_best against a config replayed from optimization_stack and, on any disagreement, overwrote current_best with the replay. The replay was a second implementation of a merge only the lift performs correctly: it unioned every entry's extra_envs without applying unset_envs, so an env a later ablation had deliberately removed came back, and resume "repaired" a correct current_best into a wrong one. With every config KEEP going through the lift, current_best and the stack are written together and cannot drift, so the comparison has nothing left to fix. The one shape still worth reporting is a config with no stack behind it. Co-authored-by: Cursor <cursoragent@cursor.com>
…eplaying _materialize_stack_config_for_resume walked optimization_stack to reconstruct cumulative args and envs, which is the merge the lift already performed on the way in. Its two remaining consumers -- the GEAK handoff's baseline_env_spec and the post-resume full-stack rebench -- now read current_best directly, so the GEAK baseline is built from the config that was actually measured rather than a replay that unioned every entry's envs and lost ablations. Despite the name, this ran on the normal path too, not just resume. What survives is the launch normalization: a "-"-prefixed key under extra_envs is a server arg, and exporting it as an env would silently drop it. That routing also had the arg merged as the full replacement string rather than an addition, so it discarded the args it was meant to extend; it now appends. values._config_from stays: it slices entries per origin and applies the publish allowlist, neither of which current_best can express. Co-authored-by: Cursor <cursoragent@cursor.com>
ZhengGong-amd
marked this pull request as ready for review
August 17, 2026 08:31
ZhengGong-amd
requested review from
Ahmedhasssan-aig,
devalshahamd and
tsrikris
as code owners
August 17, 2026 08:31
CI on the merged branch surfaced 23 failures. One is a live defect; the rest are earlier cleanup commits that deleted a symbol and left its callers or its tests pointing at it. Live defect: - The coordinator prompt's acceptance-threshold advisory called self._decaying_keep_threshold_pct(), which 27ad677 removed in favour of resolve_keep_threshold(state). The delegation entry still routed the name to the proposals collaborator, so every render of that block raised AttributeError into the prompt builder. It now calls the shared helper, and the stale delegation entry goes with it. The dead `if keep is None` guard goes too: the curve always returns a float. Dead call sites (all inside `except Exception: log.debug(...)`, so they failed silently rather than crashing, once per candidate event): - Four blocks importing write_decision_json / write_semantic_audit from framework.artifacts, whose definitions 78f162a removed. The module docstring still advertised both writers. Tests pointing at removed surfaces. Retargeted where the behaviour they cover still exists, deleted where the thing under test is gone: - collect_final replaces the recorder's _snapshot_final for the three scriptable-latency assertions. The e2el derivation is asserted through _backfill_scriptable_latency, which is where it actually happens -- on save, before any collector reads state.json. - The fusion test keeps the counts the result dict inlines and pins the absence of the kernel_sequence.json artifact; the per-cluster breakdown it also asserted only ever lived in that unread file. - The shape-contract tests assert membership in DISPATCHABLE_SHAPE_PROVENANCE, which is what the dispatch gate reads now that the wrapper is gone. - The GPU-lease test keeps its start_async / poll_started coverage and drops only the pending_seconds assertions. - The roofline audit-fields test now pins roofline_attempts and that last_roofline is absent. - Deleted: test_payload_aliases_shim.py (78f162a said the shim and its guard test were deleted; only the shim was), the three probe_ssh tests, the four infera discover_role_pods / frontend_service_url tests, the framework_levers_by_state test, the reset_specialist_dispatched test, and the two write_semantic_audit error-path tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
monkeypatch.delenv(name, raising=False) records nothing when the name is
already absent, so it does not arm an undo. The resume pass under test then
exports INFERENCEX_PATH itself, and that value -- a tmp_path that pytest
deletes -- outlived the test and reached every later one in the same process.
The victim was test_profile_executor_extracts_vllm_capture_traces, which then
resolved a dead InferenceX checkout, failed inside the executor, and returned a
failure payload with no "framework" key. It only reproduced under full-suite
ordering, and only through this one predecessor (bisected).
setenv arms the undo whatever the prior state, and the assertion is unchanged.
Two notes for whoever reads that failure next:
- ``SubAgentResult.state`` is "succeeded" whenever the executor returns at all,
including when its payload is {"status": "failed"}. Asserting state alone
does not establish that the run worked.
- ``PROFILE_DEFAULT_CONFIG`` freezes ``asset_root()`` at import time, so an
$INFERENCE_OPTIMIZER_ASSET_ROOT override that is live during the first import
of profile.py sticks for the whole process. Benign in production, where the
variable does not move; worth knowing when a path-shaped test goes strange.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Self-review of the two preceding commits. No behaviour change beyond removing code and a result key that nothing reaches. Orphaned by the decision.json / semantic_audit.json writer removal: - artifacts.candidate_slug existed to build runs/framework_agent/<slug>/ for those writers and has no production caller left (the slug helper the pump still uses is framework_agent._candidate_slug, a different function). It goes with its __all__ entry and the test file that covered nothing else. - The module logger went unused with the writers. Orphaned by the kernel_sequence.json removal: - bypass_trace_analysis still computed kernel_sequence_path and published it in the result, advertising a file that is never written and that no consumer reads. Orphaned by the pending_seconds removal: - A test double still implemented the method; nothing calls it. Redundant: - The collective promote kept an isinstance precondition on gain_per_stack_entry, which guarded a direct append the lift now owns. - _warm_kernel_keep_threshold_pct had two near-identical warning branches for one "override is unusable" case, and a function-local machine_state import where the module already imports it at the top. - Three imports left unused by the test deletions. Prose: - Comments that restated the docstring directly above them (GEMM routing) or a callee's own contract (the lift) are gone. - Docstrings that narrated what was removed rather than describing live code now state the contract; that history belongs in these commit messages. - _promote_collective_integrate_keep documents that it is a no-op on an already-stacked patch or a lift refusal, which its one-line summary implied did not happen. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ape it never renders The profile/roofline path passed --profiler-config.capture_torch_profiler True. No vLLM ProfilerConfig has ever carried that boolean: the TraceLens patch adds capture_torch_profiler_dir, a path Magpie's TraceLens route already supplies. vLLM rejected the argument outright, so every profile attempt died before the server booted, trace_analyze stayed empty, and kernel-opt had nothing to dispatch on. Drop the flag and keep the real detailed_trace_annotation. The patch sentinel matched the same phantom name as a bare substring, so it also accepted a tree holding only the upstream _dir field; anchor it on the full field name. Robustness parsed the coordinator inbox with a regex demanding payload= right after topic=, but _format_inbox_event renders a per-topic field set and delegated_result emits kind/state/error with no payload= at all, dropping msg_id when a message carries none. Nearly every row was discarded, so the agent could not see the delegated_result stream proving orchestration was alive and raised a high-severity stall alert against a healthy run. Anchor on the fields the renderer always emits, then decode the tail: payload= wins when present and the summary fields fold in underneath, which the repeated-failure signal reads. Quoted values match whole so a k=v inside a rendered error string cannot split it into a corrupt value plus a bogus key. Co-authored-by: Cursor <cursoragent@cursor.com>
…explore-opt-17
Four conflicts, all where main's new work meets this branch's current_best
convergence and its de-duplication of the journal outcome helper.
Conflicts:
- derive_journal_outcome takes main's was_skipped -> OUTCOME_SKIP branch on top
of this branch's hoisted `result` local, so the new branch reads the same
local instead of re-deriving `result_dict or {}` a fourth time.
- The fusion promote keeps _lift_to_current_best and drops target_file from its
stack entry: #1199 made the unified patch the sole target authority, resolved
from diff headers at replay. Scoped to fusion, as main scoped it -- the
collective entry keeps its target_file.
- The warm-replay promote keeps the lift and adopts main's framework_source_root
spelling for the promoted checkout, and its new workload_compatibility param.
- test_breakdown_exporter_unit takes both sides' new tests.
Mirrored onto this branch:
- #1199 renamed the promoted-checkout keys and dropped the only writer of
SharedState.active_inferencex_path, leaving its resume guard inert: the
reader, the active_inferencex_checkout_missing warning, its entry in the
terminal stop-reason registry, its operator-facing explanation in report.py
and a test asserting the restore all survive. PRELUDE writes the field again
so a resume re-points $INFERENCEX_PATH and stops when the checkout is gone.
Fixed here:
- last_roofline was removed as unread, but roofline is one of the five
_AUDIT_ACTIONS and record_action_attempt sets `last_<action>` by name. On a
plain dataclass that assignment silently created an undeclared attribute, so
the row was written in memory and dropped by to_dict on every save while its
sibling roofline_attempts persisted. Redeclared; the test now pins that both
mirrors reach to_dict.
- The recorder's sweep fragment replaced the whole section with last_sweep
verbatim, dropping all_variants -- the shaped variant points only the
collector can scan off disk. Same defect main just fixed for `session` with
_merge_session, and this branch fixed for `workload`. Here the fragment adds
nothing at all: snapshot_state_sections runs immediately after state.save, so
the collector always reads a last_sweep at least as fresh. Removed, with a
regression test that fails without it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
fd81233 removed _resolve_effective_concurrency for having no production caller and deleted test_roofline_ceiling.py with it, but that file was the module's coverage, not just that symbol's: roofline_ceiling fell to 43.50% (352 of 623 statements), the worst in the tree, and took the repo under the fail_under gate at 89.84%. Two files, 55 cases, covering 243 of those statements. The module is pure arithmetic over a config dir, so these assert the contracts rather than recorded outputs -- a golden number cannot tell a corrected formula from a broken one. PerfModel and the HF metadata it reads: - The op formulas against the arithmetic in their own docstrings: GEMM 2MNK, SDPA's two matmuls, causal halving that applies to prefill and not decode, K/V sized by the KV-head count so GQA is visible, and the MoE coupon-collector expert count saturating from topk at one token to every expert at a large batch. - Activation and weight precision stay separable, which is what keeps an fp8 checkpoint from reading its bf16 activations at the weight width. - compute_roofline_from_perfmodel: the decode rate is the slower of its own two ceilings and bound_kind agrees with it, per-op shares normalise to 1, a MoE model routes through moe_fused instead of the three dense FFN GEMMs, the hardware scales with the GPU count, and every incomplete-input path declines. - load_model_meta: the safetensors index outranks the shard sizes and a malformed one falls back, quant_method outranks torch_dtype, and the MoE decomposition charges a token only the experts it activates -- including fp4 experts under an fp8 model, whose bytes the global dtype would overstate enough to trip the safe-degrade and drop the MoE from the ceiling entirely. The diffusion arm: - VAE downscale, the latent-grid token count (FLUX's 2x2 pack, and the default when the channel ratio is not a square), and DiT geometry including dual-stream blocks counted twice and the sample_size-less path that takes its sequence length from the runtime resolution. - Both ceilings: weights read once per denoising step, linear plus attention FLOPs per image, GPU-count scaling, and the degenerate inputs that yield 0.0. Coverage: 89.90% -> 90.23% locally (7392 -> 7149 missed). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…explore-opt-17 #1194 rebuilds the optimization read model from what producers recorded rather than re-inferring ownership from phase timestamps, and reaches the same diagnosis this branch did about unmeasured gain -- from the other side. Five conflicts, all where those two meet. Conflicts: - optimizations.py drops the phase-timestamp inference block for main's recorded ownership. phase_at stays in collectors/_common: attribution.py and decision.py still share it. - exporter.py takes main's removal of the v4 canonical-stream integrity machinery, and test_breakdown_v4_core.py goes with it. This branch had touched neither; the conflict was adjacency to the alias removal above it. - schema.py restores SCHEMA_VERSION_V2 / V3. This branch dropped them as unused, but #1194 gives them a stated job -- telling a recorder-aggregated archive apart from a collector fallback -- and documents them in SKILL.md, so the premise for removing them is gone. - _promote_gemm_tuning_keep stays deleted. Main now labels what it produces measurement_basis="derived_speedup"; b4ff1b2 removed the path instead, because baseline * best_speedup is not a measurement. Every GEMM KEEP here goes through _validate_gemm_tuning_e2e, so what it stamps is an e2e_rebench. Main's new _journal_gemm_tuning_keep hook is wired into that validator, so the journalling survives the promoter it was written against. - The geak_e2e, forge-GEMM and fusion promotes keep the lift and carry main's new source= label onto the validated stamp; cumulative_gain and cumulative_gain_provenance stay removed. Mirrored onto this branch: - The collective promote keeps main's record_collective_promotion and record_session_validation calls. That lane settles its own verdict outside the kernel integrate queue, so without them its gain reports as belonging to no step -- which is the defect #1194 exists to fix. Adapted to the lift: recorded only once the winner is actually lifted, target_file resolved from the result rather than a hand-built entry, and one ts minted for the validated stamp and both recorder writes so the promotion lands on a single instant. The other paths need no wiring: #1194 put record_session_validation inside _update_cumulative_gain_validated, which every lift-based promote here calls. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The #1194 merge landed the recorder rebuild, which brought ~154 newly missed statements in with it (assembler 19 -> 86, instrument 126 -> 203, trace 10 new) and put the repo back under the gate at 89.98% -- the earlier roofline work holds, roofline_ceiling is 43.50% -> 82.66% in that same report, but a margin that thin does not survive a merge this size. roofline_ceiling, the state-level entry points the pure-function tests did not reach: - resolve_runtime_workload: the baseline yaml is the geometry of record and state attributes only fill its gaps, concurrency floors at 1, and an unreadable yaml still yields a usable workload. gpu_type comes from state or the benchmark runner_type, not from a benchmark key. - compute_roofline_breakdown_from_state on both arms: a serving run reports the lower of its two projections, an xdit run routes to the images/sec ceiling, and an unreadable model yields the empty breakdown rather than a zero that reads as measured. - The diffusion arm end to end: steps and resolution from XDIT_* with the CUSTOM_* aliases an operator workload uses, both ceilings binding together, and the memory-only degrade when the DiT transformer config is unreadable. - select_peak_and_bound treats a zero projection as unknown rather than as infinitely slow, so it cannot win the min. The assembler's fragment merge, which is what decides whether a recorded fact survives to the archive. A producer writes partial fragments from inside its own work and a second write of the same entity id merges into the first: - Nested dicts merge key by key rather than wholesale -- whole-value replacement is what drops a sibling the update never mentioned. - List rows merge on the first recognised nested id, including an id the update itself introduced; rows without one append at most once. - Filling an absent field is not a conflict and says nothing; replacing an answer with a different one is reported at its full path, last writer wins. - The versions and critic/robustness substreams fold into their sections, and a directly recorded section outranks the substreams that would rebuild it. Coverage: 89.98% -> 90.20% (7344 -> 7189 missed of 73321), leaving roughly 150 statements of headroom over the gate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
xiaofei-zheng
approved these changes
Aug 19, 2026
…lore-opt-17 ~130 commits, dominated by the cooperative-cancellation and session-clock work. It reaches the same conclusions this branch did from the other side -- "a round the run stopped is not a baseline that failed", "a variant the run reaped is not a variant that failed" -- so most of it merged clean. Five conflicts. Reversed here, deliberately: the EXPLORE hours force-exit is back fdadb65 removed DEFAULT_EXPLORE_FORCE_EXIT_HOURS_REMAINING because any run of 3h or less force-exited EXPLORE on its first tick, and argued the gate was redundant once charge-back reserves the later phases' share. 07e23b9 found the same defect and fixed it instead of removing it: _explore_hours_leavebehind_ applies disables the gate when the leave-behind is not strictly smaller than the session, with disabled_leavebehind_covers_session in the evidence and 56 lines of new tests. Main's version is taken. It closes the failure this branch removed it for, it belongs to a 130-commit effort by the owner of that machinery, and the removal rested on redundancy rather than incorrectness -- so keeping it costs a redundant check, while dropping it would delete another team's tested work mid-merge. should_force_exit_explore and exit_normal_explore carry the parameter again; a non-positive threshold still disables the gate. The other two fixes in fdadb65 are untouched: normalize_budget_pct still keeps an explicit 0.0, and the FRAMEWORK force-exit stays removed. Other conflicts: - _grid_runner __all__ takes main's three new stop-attribution exports; MULTI_NODE_DEFAULT_KEEP_THRESHOLD_PCT is not among them because 27ad677 deleted the constant, and exporting it would break the module interface. - request_handlers keeps main's vendor-playbook deploy-blocked guard and reads the stable ledger through _entry_by_kernel_id rather than the ordinal kernel_opt_attempts dict (2cb82ad). - machine_state also takes main's SWEEP closeout helper, so skip_to_close no longer maps a refused conc_sweep onto robustness_escalated. - test_phase_force_exit takes main's cases; one had spliced this branch's variable name onto main's new assertions. - CHANGELOG keeps both sides' entries. Also drops a duplicate _update_cumulative_gain_validated key this branch had left in the Coordinator delegation table (same value, so inert, but ruff F601). Not addressed here, and worth an issue against #1171: integrate_patch does not consult stop_attribution on either side of this merge, so a session-budget stop during the GEMM E2E validator files its remaining candidates as REVERT -- a verdict about tuners that were never measured. Extending that subsystem into integrate is the author's call, not a merge resolution. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The hard force-exit had two arms: the unspent fraction of EXPLORE's charge-back budget, and an absolute floor on session hours remaining. The floor was there from when a phase got a flat max_minutes*pct allotment and could therefore hold budget the session no longer had. Charge-back rebuilt the allotment from the time left when the phase starts, which made the floor redundant, and left it firing on any run shorter than the leave-behind: remaining starts at max_hours, so a 3h session was already at or under a 3h floor on the first tick. Guarding the floor against that case keeps a second gate whose every firing the budget arm reaches on its own, and which grades a phase on time other phases spent. Removed instead: should_force_exit_explore now reads the phase fraction alone, and reports fired_reasons unchanged so the evidence shape holds. The cases the guard protected are still pinned, against the surviving behaviour: a 3h session reaches EXPLORE with its budget intact both at entry and after a slow PRELUDE, and the gate fires once the phase has spent its own share. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Work the run itself stops carries one of two error classes, and the grid runner already assigns them. integrate dropped the class when it flattened the variant result, so a re-baseline the session clock or a cancel ended arrived looking like a bench that simply produced no throughput -- and every ledger downstream read that as a verdict on the patch. The class now travels with the rest of the bench result, and the two places that turn a bench into a verdict consult it first: - The perf gate returns a stopped result, with the patch and its artifacts taken back out of the tree exactly as a REVERT would, but no decision. - The kernel integrate handler no longer answers REVERT when the re-baseline produced nothing because the run ended it; the patch is reverted and the result says so without grading it. The GEMM E2E validator is the ledger this was visible in: it walks tuners one at a time and files anything that is not a KEEP as a REVERT, so a clock running out mid-sweep taught the ledger that every remaining tuner had been tested and rejected. It now stops on the first stopped round, since the cause ends the batch either way, and records nothing for the tuners it never measured. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Self-review of the two preceding commits. The stopped results carried fields that either nobody reads or that describe work that did not happen: ends_the_batch has no consumer on either result (the GEMM sweep stops on any cause, because it has no fit check to leave the rest of the batch to), and base_tput / keep_threshold_pct name a grading the round never reached. Docstrings that still promised only a verdict now say a round can end without one: the perf gate, and the GEMM sweep that stops with its remaining tuners unrecorded. The operator explanation for explore_force_exit_low_budget described the session wall-clock, which is what the removed floor measured. It is the phase's own budget that fires the gate now. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three CI failures, all tests carried in from main that name something no longer there. None of them is about the behaviour it asserts. - compute_next_phase no longer takes max_hours. Its only consumer was the FRAMEWORK hours force-exit, which went when the phase clock started charging back; the three SWEEP closeout cases pass it incidentally and assert nothing about it. - _sweep_state names the validated gain, which is the only cumulative gain left. - enablement_round_dir refuses a blank task id rather than mapping every round onto one directory. snapshot_round already skips a round that carries no id for that reason, so the fallback the test expected has no producer; the traversal case and the blank case are now the same parametrized refusal. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…eady read The stopped results introduced their own status. Two discriminators key on the established one instead, and both read the new value the wrong way: _is_integrate_fault treats "every unmeasured path returns failed" as the signal that a result is a fault rather than a verdict, so a stopped round was counted against the REVERT quota it was meant to be kept off; _is_promotable_result holds anything that is not failed to be promotable, so the same round was routed through promotion. Both stop returns now carry status failed, which is what an unmeasured round has always reported, and leave the cause to error_class -- the channel stop_attribution exists to populate, and the one the ledgers carry. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
It fabricated the status the production path no longer returns, and carried setup the assertions never depend on: a warm-replay current_best, a baseline runtime the timeout stub makes unreachable, a workspace, a precision, and two micro-speedups. Verified by defeating the guard and watching it fail. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
iraj465
pushed a commit
that referenced
this pull request
Aug 19, 2026
Resolves the two conflicts in the GEAK promote path, both created by #1188's correctness sweep landing on the same function this branch extends. phases/kernel.py, _promote_geak_from_candidate: main added a KEEP guard (a rebench that does not beat current_best must not overwrite the headline) and dropped the then-unused `provenance` parameter. This branch needs `provenance` and `overlay_loaded` to record the adoption ledger. Kept both sides: main's KEEP guard is untouched, and the two parameters return -- with `provenance` now DEFAULTED rather than required, so main's callers in writeback.py and test_geak_gain_alignment.py continue to work unchanged. loop/writeback.py: restores the two keyword arguments at the promote call sites, matching the signature above. 143 GEAK tests pass, including main's test_geak_gain_alignment.py, which omits `provenance` and so exercises the new default.
chennyiiis
added a commit
that referenced
this pull request
Aug 19, 2026
…rift The branch was cut before #1188, which deleted _promote_gemm_tuning_keep -- the synthetic-throughput promoter this branch had been carefully teaching not to stamp self-reported speedups as validated. Rebasing brings that deletion back, so the two hunks guarding it and the two tests covering it are gone; #1188 solved the same problem the stronger way, by measuring every candidate end to end instead of declining to certify a ratio. What survives is the layer above it: an end-to-end number can be measured correctly and still be measuring the wrong thing, because the tuned table may never have been read. The apply-verification test module skipped on the top-level forge package while production imports forge_gemm_tune.evidence. A box with the package but not that submodule passed the skip, got a None parser, scored every verdict 'unknown', and failed eleven cases for reasons unrelated to what they test. Now 44 pass and 14 skip without forge, 58 pass with it. The env-var-to-table map existed twice in this file, and a name that drifts makes the apply check compare our deployed file against the wrong table, conclude the artifact never arrived, and revert a candidate that was fine. One constant now, with a test pinning the third copy in KernelForge wherever it is importable. AITER_LOG_TUNED_CONFIG and HYPERLOOM_GEMM_PAIRED_PAIRS are documented. The first changes the environment of every serving run, so an operator wanting to turn it off has to be able to find out it exists. Co-authored-by: Cursor <cursoragent@cursor.com>
ZhengGong-amd
pushed a commit
that referenced
this pull request
Aug 20, 2026
…tuned-config hit logging (#1222) * fix(gemm): stop stamping self-reported speedups as validated, log tuned-config hits Three changes on the measurement-trust side of GEMM tuning. _promote_gemm_tuning_keep derived tuned_tput = baseline * speedup from an externally reported number and wrote the result straight into cumulative_gain_validated. The comment above it already said the figure was "inferred from a micro-benchmark's speedup, never measured end to end" and stamped it anyway. On this fleet three rounds of one unchanged configuration span 58%, so a lone reported speedup > 1.0 is within what drift alone produces. The entry is still promoted -- the artifact is real -- but the gain is now recorded as reported, not validated, unless the result carries an end-to-end number we measured, in which case that number is used directly instead of being reconstructed from a ratio. The e2e candidate filter accepted status "ok" plus candidate-or-improved_shapes. improved_shapes cannot exceed 0 for tuners that never time an untuned baseline (TunableOp, the candidate-CSV fallback, hipblaslt-only bf16), and those now report unverified_shapes; partial_output is likewise a real artifact whose rows are deployable. Both are accepted so the forced-e2e path is not undone here. AITER_LOG_TUNED_CONFIG is set for every serving run. aiter logs tuned-config misses unconditionally but hits only behind this flag, and a scan of 60 production server logs found it set in none of them -- which leaves "0 hits" and "hit logging was off" indistinguishable, and makes the GEMM shape list fall back to config.json, which covers 0.4% of real lookups. The bypass backend launches its own server and forwards almost nothing, so it carries the flag explicitly. Adds orchestrator/measurement/convergence.py: discard the warm-up round, then require the remaining rounds to agree before a throughput counts as steady. A controlled repeat of five identical passes spanned 117.6%, and 3.9% once the cold round was dropped, so the fix is to discard it rather than to loosen the threshold. Not yet wired into the measurement rounds. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(gemm): record how steady the baseline anchor is, instead of assuming it The double-run discards round 1 by design, which leaves exactly one usable measurement -- and one measurement cannot be shown to be steady. The whole gain ledger is then graded against that number with a 3% KEEP threshold, while a real session produced 14,202 -> 19,374 -> 22,425 tok/s from one unchanged configuration. So the convergence verdict is now recorded on the result (it reads insufficient_rounds, which is the honest answer for two rounds where the first is known-cold; establishing convergence needs a third), together with the cold-start delta. A warning is raised only when that delta is large enough to suggest round 2 had not settled either. Deliberately does not fail the baseline: halting here would stall the session, and the point is to make the anchor's provenance visible to the gates that grade against it. Co-authored-by: Cursor <cursoragent@cursor.com> * feat(gemm): add paired A/B and tuned-artifact apply verification Two judges, both deliberately unwired for now: they are the pieces the KEEP gate is missing, and landing them separately keeps the behavioural change reviewable. paired.py -- convergence establishes that each side is steady; it does not establish that both sides saw the same machine. Measuring all of A then all of B leaves drift between the blocks indistinguishable from the effect. One resident server held ~137 req/s for three identical passes and then fell to 117 when a neighbour landed, a 16% swing owing nothing to the configuration. So pairs are interleaved and judged on the median paired difference, and pairs that disagree in sign report inconclusive rather than letting an average invent a winner. apply_verification.py -- artifact exists, env var set, throughput up, and the tuning can still have done nothing: the keys may be unreachable, or the merge step may never have picked the table up and the server loaded its bundled default. Neither is detectable by choosing a better tuner. The trap this had to avoid: aiter logs a miss unconditionally but a hit only under AITER_LOG_TUNED_CONFIG. Reading "no hit lines" as "zero hits" would revert every arm that ran without the flag -- in a scan of 60 production logs, all of them. "Cannot tell" is therefore its own verdict, and only "not merged" and "zero hit" block a KEEP. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(gemm): refuse a KEEP whose tuned artifact provably never applied The e2e loop already computed whether the tuned table was reachable, then logged the answer and credited the gain anyway. A run whose artifact the runtime never read measures drift, not tuning, so both positive findings now block the promotion: the shape keys never resolving (coverage) and the table never reaching the server's merge list (apply verdict). "Cannot tell" still does not block -- aiter only logs a hit under AITER_LOG_TUNED_CONFIG=1, and a scan of 60 production logs found it set in none of them, so reading a missing hit line as a failed apply would revert every arm. Also wires the paired A/B judge, which until now no caller used. base_tput and new_tput are measured at different times, so their difference carries whatever the machine did in between -- 16% on one controlled repeat on this fleet, 58% across three rounds of one unchanged configuration. Interleaving separates the two but costs two extra rounds per pair, so it is opt-in via HYPERLOOM_GEMM_PAIRED_PAIRS; what is no longer optional is saying which of the two produced the number, so a block comparison cannot arrive downstream as an unqualified "validated". Verified on an 8-GPU MI355X box: a real vLLM run with the injected flag emitted 83 tuned-config hits against 122 misses and the parser called it "served" -- the first non-empty apply verdict available, since the flag was previously set nowhere. 164 tests pass, 10 of them new. Co-authored-by: Cursor <cursoragent@cursor.com> * test(gemm): refresh the env golden locks for the aiter hit-logging flag Every serving run now materializes AITER_LOG_TUNED_CONFIG=1, which is the whole point of that change -- aiter logs a tuned-config miss unconditionally but the matching hit only under the flag, and both GEMM shape discovery and apply verification read hits. The three golden snapshots still pinned the YAML from before it, so all four CI shards failed on them alone. Its position differs between the snapshots because a caller passing extra_server_args seeds EXTRA_*_ARGS earlier in the dict, so the flag lands after it in the two baseline snapshots and before it in the profile one. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(gemm): repair the measurement package files and the stale validated-gain assertion Two leftovers from earlier commits on this branch, both caught by CI shards 3 and 4: * `measurement/__init__.py` and its tests' `__init__.py` were written through a shell that neither interpreted the escapes nor omitted the BOM, so the module docstring arrived as one line of literal backtick-n behind a U+FEFF. The LLM architecture guard walks every source file and cannot parse one, so it failed the whole shard rather than the file. * `test_kernel_entry_auto_runs_gemm_tuning_for_fp8_sglang` still asserted that a self-reported micro speedup lands in `cumulative_gain_validated`. That is the behaviour 563eb1d deliberately removed: the 1.28x is the tuner's own figure, and reconstructing tuned_tput from it is not a measurement we made. The reported gain is still recorded; only the validated stamp waits for an end-to-end number of our own. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(gemm): keep the per-tuner failure signal in the tuning audit row `reports/trace/gemm_tuning.jsonl` exists to answer one question -- did this tuner run -- and recorded only `tuner`, `best_micro_speedup` and `kept`, none of which can separate a tuner that crashed from one that searched and found nothing. The `result.json` beside it held `status`, `elapsed_s`, `error` and `error_class`, and all four were dropped on the way in. The envelope reported no `error_class` either, even when a tuner had named one. Replaying the campaign's own artefacts under `/shared_nfs/hyperloom-claw` through the writer, before and after: 349 result.json, 378 tuner runs, of which 43 ended `failed` or `empty_output`. The old row made 0 of those 43 visible; the new row makes all 43 visible, and 28 envelopes gain an `error_class` a tuner had already supplied. What appears is diagnostic rather than noise -- e.g. a `sglang_dense_bf16` row that now reads `status=failed`, `error_class=validation_error`, `intermediate_size not set in model config` instead of three nulls. This is the blind spot that hid #1211: 82 runs rejected by argparse in ~11s and recorded as a clean `no_improvement`, indistinguishable in the trace from the six that really tuned for ~1600s. It also matters going forward, because the tuner now emits finer outcomes than it used to (`partial_output`, `unsupported_argument`, dropped-as-inaccurate rows) and this row is where those distinctions were being thrown away again. A clean run stays compact: everything added is dropped when null, so a successful row gains only `status` and `elapsed_s`. `tuner`, `best_micro_speedup` and `kept` are still emitted even when null -- `kept` is null on every row observed so far, and an absent key would be indistinguishable from `false`. `error` is truncated to 400 characters; it is a pointer, and the full text stays in the run's own result.json and tune.log. Closes #1210. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(gemm): make both blocking apply verdicts reachable and correct Review findings on the KEEP gate. Both defects pointed the same way: the gate looked complete and could not actually fire, or would fire on everything. **`zero_hit` was dead code.** `BLOCKING_VERDICTS` lists it, but the parser answers `inconclusive_no_hit_logging` for every `hits == 0 and misses > 0`, which is the exact condition `verify_applied` tests before returning `zero_hit`. The branch was unreachable, so one of the two blocking verdicts did nothing. That matters more now than it would have before: this same PR makes `AITER_LOG_TUNED_CONFIG=1` the default for every serving run, and with the flag on, "0 hits and N misses" is a genuine zero -- precisely what the gate exists to catch -- yet it was being waved through as "cannot tell". The caller knows whether the flag was set, so it now says, and unknown still means inconclusive. **The arrival check compared the wrong names, and would have reverted everything.** It required our artifact's basename in aiter's merge line, but by then the env value is `merged_<candidate>.csv`, a name Hyperloom invented, while aiter's merge line lists canonical table names. Worse, on a real MI355X run the comparison never ran at all: setting `AITER_CONFIG_*` -- what every candidate does -- makes aiter skip the merge step entirely, print no merge line, and name our file directly in the lookup lines. So the only branch that can revert every candidate is the one the live verification never reached, and the unit test constructed a log where the basenames happened to line up, pinning the assumption instead of testing it. Arrival is now judged on the tables the lookups actually named, which is where the evidence is in both modes, and accepts either the candidate's filename or the canonical table name the runtime resolves it under. It only blocks when none of the deployed artifacts appear anywhere the runtime looked. **The paired confirmation served its B leg differently from the KEEP it was confirming.** `fmoe_ck` only takes effect under `--moe-runner-backend aiter`; the confirmation payload carried no server args, so B ran without the CK path active, never read the tuned table, and measured the same thing as A. A real gain would come back `within_noise` and be labelled unconfirmed. **The apply-verification tests never ran in CI.** `importorskip ("forge_gemm_tune")` skipped the module, and forge is not a Hyperloom dependency, so the gate's whole decision surface had no automated coverage -- 9 tests, all skipped. They are parametrised over a stand-in parser now, so the verdict logic is tested everywhere, and over the real parser as well wherever forge is installed. Both agree on every case: 42 pass without forge, 55 with. Also: a missing `server.log` is logged rather than silently returning None (the one way this check can fail quietly), and `has_e2e_evidence` is documented as a forward-looking contract with no producer today. 541 tests pass. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(gemm): warn when MoE routing is coarser than the log it read Routing a vLLM run to the aiter tuner family is a substring scan: an aiter fused-MoE marker anywhere sends the whole run that way, and `vllm_moe_triton` then never runs. A run can dispatch both -- aiter CK over part of the token range, vLLM's Triton path over the rest -- and forge's own parser records that case as `impl="mixed"`. Whichever way the single flag falls, the range the other backend served is left untuned. It is the same shape as the defect already fixed one level down, where a single 1-stage sighting disabled CK tuning for the tokens 2-stage was serving. Reported rather than acted on: changing this routing changes which tuners run for every aiter-served vLLM model, which is a larger step than it looks and does not belong in the same change as the checks around it. Forge already adds `fmoe_ck` from the same evidence, so what this names is the Triton half of a mixed run -- the part still going untuned, and now visible instead of implicit. Best-effort throughout: no forge installed, an unreadable log or a parse failure all leave routing exactly as it was. 522 tests pass. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(gemm): rebase onto the e2e validator, and stop three sources of drift The branch was cut before #1188, which deleted _promote_gemm_tuning_keep -- the synthetic-throughput promoter this branch had been carefully teaching not to stamp self-reported speedups as validated. Rebasing brings that deletion back, so the two hunks guarding it and the two tests covering it are gone; #1188 solved the same problem the stronger way, by measuring every candidate end to end instead of declining to certify a ratio. What survives is the layer above it: an end-to-end number can be measured correctly and still be measuring the wrong thing, because the tuned table may never have been read. The apply-verification test module skipped on the top-level forge package while production imports forge_gemm_tune.evidence. A box with the package but not that submodule passed the skip, got a None parser, scored every verdict 'unknown', and failed eleven cases for reasons unrelated to what they test. Now 44 pass and 14 skip without forge, 58 pass with it. The env-var-to-table map existed twice in this file, and a name that drifts makes the apply check compare our deployed file against the wrong table, conclude the artifact never arrived, and revert a candidate that was fine. One constant now, with a test pinning the third copy in KernelForge wherever it is importable. AITER_LOG_TUNED_CONFIG and HYPERLOOM_GEMM_PAIRED_PAIRS are documented. The first changes the environment of every serving run, so an operator wanting to turn it off has to be able to find out it exists. Co-authored-by: Cursor <cursoragent@cursor.com> --------- Co-authored-by: Cursor <cursoragent@cursor.com>
iraj465
pushed a commit
that referenced
this pull request
Aug 20, 2026
Review B5. `_promote_geak_from_candidate` already takes `overlay_loaded` and already refuses to credit unproven kernels in the per-kernel ledger. The stack entry was the other reader and did not make the same call: it copied GEAK's self-reported `accepted_kernels` / `accepted_heads` unconditionally. `_geak_contribution` classifies the dashboard row from those lanes alone, so a rebench that stripped a dead overlay and promoted on its config gain was filed under `kernel` while the ledger correctly said unattributable. Now the lanes travel only with the proof, and `_geak_contribution` treats `overlay_loaded is False` as proof of absence whatever the lanes say. Entries written before the stamp existed have no key at all and are left to the lanes, so no historical row is reclassified. Also drops the dead `shared_state.cumulative_gain` write: #1188 (c92784c) removed the field from `SharedState`, and `_update_cumulative_gain_validated` on the next line is the sole writer of the gain ledger. Two unit tests: promote with a dead overlay leaves no kernel name on the entry and classifies as `config`; promote with a proven overlay keeps the name and classifies as `joint`.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This branch is an end-to-end audit pass over the orchestrator. It started from
one question — is every number the loop promotes actually measured? — and the
answer pulled in four adjacent areas: the promotion path into
current_best,the phase budget gates, the resume/session identity path, and the benchmark
environment contract. Along the way it removes ~10k lines of code that nothing
reaches: an undeployed robustness-server data path, an orphaned kernel harness,
and a long tail of symbols, artifacts and state fields with no reader.
The net is negative by 8k lines, but this is not a cleanup PR. Roughly half
the commits fix live defects, several of which silently corrupted the session's
reported gain. The removals are what made those defects visible.
1. Measurement integrity: nothing enters
current_bestunmeasuredThe central defect class: several paths wrote a throughput onto
current_bestthat no benchmark ever produced, which then raised the grading anchor for every
later candidate and rewrote the session's reported gain.
b4ff1b297— GEMM tuning KEEPs are now measured end-to-end. The GEAKbranch derived a throughput as
baseline_tput * best_speedupand wrote itstraight onto
current_best. A GEMM speedup of 1.1 would overwrite acurrent_bestalready holding an explore win of 1.5×, and rewrite thesession's validated cumulative gain as that single GEMM's speedup. Both
result shapes now reduce to a common candidate list and route through the
per-tuner validator (previously forge-only), which measures each candidate
against
current_bestviaintegrate_handler.045f7d08b— profiler-on measurements no longer promote. A profile runbenches with the torch profiler attached, so its throughput is not comparable
with un-profiled measurements. Promoting it on a private +1% rule bypassed the
anchor gate, inflated the anchor for every later candidate, and left no
optimization_stackentry — so the stale-stack warning never fired and agenuinely better KEEP measured below the inflated anchor was silently refused.
227471e3d— one writer forcurrent_best.integrate,geak_e2e,forge GEMM, fusion and
warm_replayeach hand-built their owncurrent_bestand stack entry; four replaced
current_bestoutright, so a KEEP could dropcumulative server args or resurrect an env an earlier ablation had removed.
All five now hand a winner dict to
_lift_to_current_best. Two behaviourchanges fall out:
integrateandfusiongain the anchor guard they lacked(a KEEP measured below the established recipe is now refused rather than
lowering
current_best), andwarm_replaystops hand-appending a roundedpercentage to
gain_per_stack_entry.cc9f8843f,82318eabe,6389b5df4— the stack replay is gone. Resumerebuilt
current_bestfrom anoptimization_stackreplay and overwrote it onany disagreement. The replay unioned every entry's
extra_envswithoutapplying
unset_envs, so resume "repaired" a correctcurrent_bestinto awrong one. With every config KEEP going through the lift, the two are written
together and cannot drift. The GEAK handoff and post-resume rebench now read
current_bestdirectly; the launch normalization that survives also had abug where a
--prefixed env key routed to server args replaced the argstring instead of appending to it.
de10702d2—target_reachedgates on the validated gain.TargetGainObjectivereadcumulative_gain, which any promotion could movewithout a stack rebench behind it, so a run could stop on a number no
full-stack measurement supported. Every other loop decision already read
cumulative_gain_validated.eee047f26,b81dc3b35,cc30655cb—cumulative_gainremoved. It andcumulative_gain_validatedcomputed the same expression; the only way theycould differ was a write path recording a gain it should not have. The
documented "per-round sum" never existed in any revision. One of the three
removed "validated or raw" fallbacks published the raw value under the key
validated_e2e_gaininto the cross-session recipe KB whenever the validatedgain was exactly
0.0— a legal result, not a missing one.beadc4c8a— a refused promotion journals asno_promote. The lift candecline a KEEP the executor granted; the audit row said
no_promotebut thejournal keyed only on executor status and wrote
KEEP, so a session auditcounted an adoption that never happened.
2. Phase budgets: stop evicting phases that have done no work
fdadb6500fixes three defects that let a phase be skipped or starvedwithout spending any of its own budget:
normalize_budget_pctdropped0.0as missing, so the sentinel written fora disabled phase was overlaid with the library default and its revived share
stayed in the charge-back denominator — shrinking exactly the phases the
redistribution was meant to fund. Measured: under
--no-kernel, EXPLORE wasallotted 324.8 min where 498.8 min was intended.
any run of 3 h or less force-exited EXPLORE on its first tick with
phase_remaining_pct == 1.0. The surviving budget-fraction arm reproducesevery long-run case the hours arm caught.
session total, so every macro-cycle reloop into FRAMEWORK — the preferred
reloop target — entered and force-exited on the same tick.
--explore-force-exit-hours-remainingfrom the 3 h demo, whichcould only run because it disabled the gate.
504bb245dputsframework_agentKEEP on the decaying threshold curve;_materialize_approved_proposalreturned before the injection ran, so italways fell back to the static 1.0% default.
27ad67737routes all four dispatch sites through oneresolve_keep_threshold(state), replacing anexcept Exception: passwrapperand a baked-in literal dict. Prompt copy no longer states a number that drifts
from the live curve.
3. Resume and session identity
8f5532572—TERMINAL_STATESderived from the transition table. Thefailed -> runningedge was removed when retries moved to a fresh idempotencykey, but the hand-listed set kept its old membership, so a
failedrowreported as still live. Three guards wedged on this: the watermark roofline
gate never released (and the marker is persisted, so the next resume inherits
the wedge), resume recovery never cleared
enablement_validation_pending, and the FRAMEWORK authoring pump re-selecteda dead candidate every tick.
c29169601— never roll back a pending integrate on an unreadable eventlog. "No KEEP found" and "could not read the event log" were the same
outcome, so a transient DB error during the first resume after a hard crash
would reverse-apply every recorded patch, discard the attempt venv, report the
loss as a fix, and clear the sentinel so no later resume could retry.
2d7667a02— session dirs are unique within a UTC second. The name is thede-facto session id (it feeds KB fact writes, critic requests and per-session
sinks), so a same-second collision merged two runs' identity, not just their
artifacts.
493e801db— bare--resumeremoved (BREAKING). It scanned every modelsubtree under
$USER_DATA_PATH, attached to the newest timestamp, andre-exported that session's model as
MODEL_PATHwithout a word — silentlycontinuing someone else's run on a different model, which
SKILL.mdalreadyforbids.
--resume-fromis the only entry point;c70e31548re-anchors thecrash-recovery monitor,
7c13a720apins that legacy command lines exit 2(argparse resolves
--resumeas a prefix of--resume-from), and7f615df2bfixes the docs that documented timestamp-guessing in the same file that
forbade it.
ab9568ca7— a relative$USER_DATA_PATHis absolutised at the launcherboundary. Subprocesses spawn with their own cwds, so parent and child
resolved different workspace roots while both believed they agreed.
a4620b32f,78189a8ea,b9a4b09d5— blank path-id components arerefused outright instead of mapping onto a shared
unknownbucket that had noproducer and forced an early return past the traversal check.
4. Benchmark environment contract
76b03786b— the bypass server gets the materialized envs. The bypassserving path hand-picked
ROCR_VISIBLE_DEVICESout ofbenchmark.envsanddropped the rest, so a candidate whose only difference from baseline was an env
launched a byte-identical server — same argv, same environment. The
measured delta was run-to-run noise, the 1% keep threshold promoted it, and the
env landed in
current_bestand in the reference script handed to the user.Kernel bundles and framework levers are activated by exactly such switches, so
their attribution was measuring nothing. Behaviour change: configs carrying a
static
PATHunderbenchmark.envsnow have it reach the server, matchingMagpie.
8aa97021d— per-variant env overrides hardened.GridVariant.extra_envswas filtered for control-plane credentials only, so a proposed variant could
carry
LD_PRELOAD/PATH/PYTHONPATHinto the benchmark config — inertwhile bypass exported nothing, a live injection surface the moment it does.
An allowlist isn't usable (grids legitimately set
CONC/ISL/TP/…), soshell and loader hijacks plus control-plane credentials are blocked instead.
6e0740cdf—unset_envscannot drop pinned workload envs. It popped anyname given, so a variant could remove TP, CONC, ISL, RUN_EVAL or the GPU mask.
That doesn't toggle a knob, it retargets the benchmark — dropping TP also
shrinks the Ray lease to a single GPU, since the lease size is read back out of
benchmark.envs.8167b8b72— the breakdown recorder stopped dropping workload fields. Arecorder fragment replaces its whole section rather than merging, and the
workload fragment wrote
frameworkwhere both renderers expectframework_name, omittedframework_versionandobjective, and coercedunset
tp/conc/isl/oslfromNoneto0(which reads as a real value). Therecorder is on by default.
5. Robustness agent
e01bbcca1(BREAKING) — the remote cluster data path is removed. Norobustness-server is deployed,
ROBUSTNESS_SERVER_URLis never set, and thefive workload-uid env keys the hierarchy lookup depends on have read sites but
no writers. The client was GET-only with no registration endpoint, so the
server could not have learned this session either way — yet
Config.discover()paid two doomed health probes every tick. Removed with it: the Source
adapter, the cluster payload decoder, and the
cluster_fault/pod_not_runningsymptoms that had no other producer. Four CLI flags go withit (
9d700952a).ReactorBundle.aclose()now closes the RCA engine's HTTPclient, which was never released.
9d700952a— multi-node runs get the real agent, not the heartbeat mock.The downgrade guarded against LocalProbe false positives, but
disable_local_probealready defaults to True on multi-node and swaps in asilent stub — so the downgrade was discarding every signal read straight off
the Coordinator prompt and inbox, which cannot false-fire on a worker node.
Multi-node produced no robustness symptoms at all, including
deadline_imminent, the one that drives thedelegate(report)wind-down.Expect alerts where there were none;
--robustness-mockrestores the oldbehaviour.
95d4646f0— a tick that harvested nothing no longer counts as a healthyprimary, so the router degrades to the local probe instead of reporting a clean
bill of health.
96d89e51e— the runtime CLI fallback that backfilled a shadowsession_idrebuilt the context by hand-listing members; that list wascomplete at 7 fields and the snapshot has since grown to 19, so a tick taking
the fallback silently lost
stop_reason, the wall-clock budget, validatedgain, tick and macro_cycle — muting postmortem finalization and the deadline
wind-down.
b70177075— the SKILL.md symptom table listed ~20 of the 52 symptoms therules emit. It is now a complete inventory keyed on the
SignalSpec.nametheclassifier and factory both read (verified by set-comparison against the
name=literals insignals/), so it cannot drift.cd7616103drops the sixfamilies of milestone codes that said when a feature was built rather than
what it does — three of which pointed at nothing.
6. Dead code and unread artifacts
02162bce8retiresparallel_e2e_runner.py(538 lines), theself-validation harness from before a KERNEL phase existed. Its baseline step
was removed in May and
--backendsdefaults empty, so a plain invocation onlyraised. Its deletion also closes an aggregation bug rather than fixing it: every
attempt's exception flattened to a row-level
failedwhile the aggregateprinted
"status": "succeeded"and returned 0 (measured: 2/2 failed, exit 0).Its
load_env_fileduplicatedray_runtime.py's credential-alias derivation,which stays and is pinned by seven tests.
78f162aferemoves four categories of write-only output: the criticevents.jsonlaudit trail (7 call sites, no reader),framework_agentdecision.json/semantic_audit.json(not in the session package glob, noskill points at them),
bypass_trace_analysiskernel_sequence.json(alreadyinlined in the result), and the
extra_sglang_argsshim.fd81233c8deletes 15 symbols with zero production callers, including theknowledge_planetyped read/write wrappers the codebase never adopted.4b1ee80ddremoves three reads of attributesSharedStatedoesn't define,each absorbed by a
getattrdefault — most consequentially, the frameworkranker prompt read
best_throughput/baseline_throughputand thereforenever told the model the current best throughput. It now shares
resolve_grading_anchor_tputwith the admission path.2cb82ad7ccollapses the ordinalkernel_opt_attemptsindex onto thestable
kernel_opt_task_attemptsledger; the two diverged through threeasymmetric cleanup paths and consumers fell back between them in inconsistent
directions. A
SharedStateproperty keeps existing test doubles working.212e6c8abconverges genuinely duplicated helpers: the lease-reap +task-reclaim + DB-retention sequence (copied verbatim between the maintenance
tick and the cycle soft-restart), five Claude-SDK message-text extractors with
slightly different gaps, and three identical
_phase_atbinary searches. Fourother near-duplicates are left merged-in-comment-only with the reason recorded
(differing error contracts, undocumented threshold differences, and one that
must stay importable on remote nodes without the
hyperloompackage).fa264d8e6,473aa8c8f,5d49d0fdb,e509017d3clear the remainingunread aliases, stale mirrors and follow-up breakages.
e509017d3is a reviewpass over the branch itself and fixes two defects the earlier mechanical stages
introduced (an
AttributeErroron every GEMM-Roofline state sync, and adouble-seeded plateau test).
Breaking changes
--resumeremoved--resume-from <session-dir>; stale invocations exit 2--robustness-server-url,--robustness-workload-uid,--robustness-enable-cluster-pod-metrics,--robustness-pod-metrics-categoriesremoved$ROBUSTNESS_SERVER_URL,$ROBUSTNESS_ENABLE_CLUSTER_POD_METRICSno longer readcluster_fault,pod_not_running,pod_no_metricssymptoms removed--robustness-mock--robustness-mockexplicitly for the old behaviourparallel_e2e_runner.pyremovedci-e2e.ymlsession_breakdown.jsonlosesfinal.cumulative_gain_pct_per_round_sumcumulative_gain_pct_validated— it was equal in every session current code can producebenchmark.envsPATHin a shipped config now wins over the caller's inherited value, matching MagpieLegacy
state.jsonfiles load unchanged —from_dictdrops unknown keys.Reverted in-branch
dd163f2c9recorded two blockers (an unwired MoE guard incompose_server_args,and
stop_ray_if_ownedhaving no production caller so no path stops a Ray headthis process started). It was reverted in
e39b39bbf— both findings stand butbelong in the issue tracker, not the changelog.
Review notes
The commits are ordered to be readable one at a time and each is
self-justifying;
e509017d3is a review of the rest of the branch and is worthreading first for the list of what the earlier stages got wrong. The largest
single-file diffs (
phases/kernel.py,loop/writeback.py,phases/machine_state.py) are dominated by §1 — the convergence onto onecurrent_bestwriter — and are best reviewed against227471e3dandb4ff1b297together.